Description

get_github_app_slug() reads github-app-id from config_.root_data, bypassing
Config.get_value() precedence (repo/local overrides). In deployments where different repositories
use different GitHub App IDs, this can cache/return the wrong slug and generate an incorrect
app_bot_login, breaking bot-PR detection.

Code

webhook_server/utils/github_repository_settings.py[R453-456]

+    github_app_id: int = config_.root_data["github-app-id"]
+
+    if github_app_id in _github_app_slug_cache:
+        return _github_app_slug_cache[github_app_id]

Relevance

⭐⭐⭐ High

Team repeatedly enforces config override precedence via Config.get_value/extra_dict (accepted in PRs
#1109, #1112).

PR-#1109
PR-#1112

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The new slug helper keys caching off root_data, but the Config abstraction explicitly supports
repository and local overrides and is used elsewhere for github-app-id, so bypassing it can yield
a mismatched app id/slug pairing.

webhook_server/utils/github_repository_settings.py[446-456]
webhook_server/libs/config.py[132-153]
webhook_server/libs/github_api.py[878-883]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`get_github_app_slug()` uses `config_.root_data["github-app-id"]`, which ignores config precedence (repo-level and repo-local overrides). This can make the slug cache key incorrect and return the wrong slug for repositories that override `github-app-id`.

## Issue Context
Other parts of the codebase retrieve `github-app-id` via `Config.get_value(..., extra_dict=repository_config)`, indicating per-repo overrides are supported/expected.

## Fix Focus Areas
- webhook_server/utils/github_repository_settings.py[453-456]

## Suggested fix
- Replace `config_.root_data["github-app-id"]` with an override-aware resolution, e.g. `config_.get_value("github-app-id")` (and/or thread the relevant repo-local `extra_dict` into this helper if needed).
- Ensure the cache key matches the resolved/effective app id for the current repository context.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Description

The new app_bot_login tests configure mock_api.get_user() but never assert it is not called
during slug-based initialization, so a regression reintroducing GET /user could still pass. This
weakens the guarantee that installation-token paths do not rely on get_user().

Code

webhook_server/tests/test_github_api.py[R584-601]

+        mock_api = Mock()
+        mock_api.rate_limiting = [100, 5000]
+        mock_user = Mock()
+        mock_user.login = "test-user"
+        mock_api.get_user.return_value = mock_user
+
+        mock_api_rate_limit.return_value = (mock_api, "TOKEN", "USER")
+        mock_repo_api.return_value = Mock()
+        mock_repo_github_api.return_value = Mock()
+        mock_get_apis.return_value = []
+        mock_repo_local_data.return_value = {}
+
+        headers = Headers({"X-GitHub-Event": "unsupported_event"})
+        webhook = GithubWebhook(hook_data=pull_request_payload, headers=headers, logger=Mock())
+        webhook.app_bot_login = ""
+
+        await webhook.process()
+        assert webhook.app_bot_login == "manage-repositories-app[bot]"

Relevance

⭐⭐⭐ High

They frequently accept negative-call assertions to prevent regressions (e.g.,
assert_not_called/not_awaited in PRs #1007, #1036).

PR-#1007
PR-#1036

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

PR Compliance ID 2 requires tests to validate slug-based bot login formatting and to fail if
get_user() is invoked in the installation-token context. In the added tests, mock_api.get_user
is configured, but there is no assertion that it is not called, so a regression could still pass.

Update and add tests to cover app_bot_login initialization via GET /app and bot login formatting
webhook_server/tests/test_github_api.py[584-589]
webhook_server/tests/test_github_api.py[600-601]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The new tests for `app_bot_login` initialization don’t fail if `get_user()` is invoked, because they set up `mock_api.get_user.return_value` and don’t assert `mock_api.get_user.assert_not_called()` (or similar). This leaves the rule “no GET /user for installation tokens” unprotected against regressions.

## Issue Context
Per compliance, installation-token authentication must avoid `GET /user`. Tests should explicitly fail if `get_user()` is called in the slug-based initialization path.

## Fix Focus Areas
- webhook_server/tests/test_github_api.py[564-683]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Description

The new async tests pass mock_repo_api and mock_repo_github_api in the opposite order of their
stacked @patch(...) decorators, so the return values are configured on the wrong patched
functions. This makes the tests brittle and can silently stop validating the intended behavior (or
mask unintended behavior) depending on future changes to the mocked call sites.

Code

webhook_server/tests/test_github_api.py[R564-580]

+    @patch.dict(os.environ, {"WEBHOOK_SERVER_DATA_DIR": "webhook_server/tests/manifests"})
+    @patch("webhook_server.libs.github_api.get_github_repo_api")
+    @patch("webhook_server.libs.github_api.get_repository_github_app_api")
+    @patch("webhook_server.libs.github_api.get_api_with_highest_rate_limit")
+    @patch("webhook_server.utils.helpers.get_apis_and_tokes_from_config")
+    @patch("webhook_server.libs.config.Config.repository_local_data")
+    @patch("webhook_server.libs.github_api.GithubWebhook.add_api_users_to_auto_verified_and_merged_users")
+    @patch("webhook_server.libs.github_api.get_github_app_slug", return_value="manage-repositories-app")
+    async def test_process_initializes_app_bot_login_from_slug(
+        self,
+        mock_get_slug: Mock,
+        mock_auto_verified_prop: Mock,
+        mock_repo_local_data: Mock,
+        mock_get_apis: Mock,
+        mock_api_rate_limit: Mock,
+        mock_repo_api: Mock,
+        mock_repo_github_api: Mock,

Relevance

⭐⭐⭐ High

Team often accepts test-mocking correctness fixes in this file (e.g., patching/to_thread mocking
fixes in PR #906).

PR-#906

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The decorator stack patches get_github_repo_api above get_repository_github_app_api, but the
test function signature lists mock_repo_api before mock_repo_github_api. With stacked patches,
this reverses which patched function each parameter corresponds to, so the test sets return values
on the wrong patched call sites.

webhook_server/tests/test_github_api.py[564-582]
webhook_server/libs/github_api.py[47-61]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
The stacked `@patch(...)` decorators inject mocks in reverse order (bottom-most patch becomes the first argument). In the new tests, the parameters for the top two patches are swapped (`mock_repo_api` vs `mock_repo_github_api`), so the test configures the wrong mock objects.

### Issue Context
This affects at least these new tests:
- `test_process_initializes_app_bot_login_from_slug`
- `test_process_app_bot_login_skips_when_already_set`
- `test_process_app_bot_login_handles_slug_failure`

### Fix Focus Areas
- webhook_server/tests/test_github_api.py[564-660]

### Expected change
For each affected test, swap the order of the last two injected mocks so it matches the decorator stack (i.e., `get_repository_github_app_api` mock param should come before `get_github_repo_api` mock param), and ensure the `return_value` assignments match the corrected param names.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Description

The updated tests set both mock_config.root_data and mock_config.get_value.return_value but
never assert that get_value("github-app-id") is called, so a regression back to reading
root_data["github-app-id"] directly would still pass. This weakens test coverage and regression
protection for the newly changed config access behavior.

Code

webhook_server/tests/test_github_repository_settings.py[R726-730]

        mock_config = Mock()
        mock_config.data_dir = "/test/dir"
        mock_config.root_data = {"github-app-id": 12345}
+        mock_config.get_value.return_value = 12345

Relevance

⭐⭐ Medium

Mixed history: extra negative call asserts accepted (#1036) but “tighten assertion” suggestions
often rejected (#994).

PR-#1036
PR-#994

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

PR compliance requires new or changed behavior to be covered by tests, but while these tests now
configure mock_config.get_value.return_value, they also still provide `mock_config.root_data =
{"github-app-id": 12345} and only validate outcomes (e.g., Auth.AppAuth(app_id=12345, ...)`) that
would succeed whether the value came from get_value() or direct root_data access. Because there
is no assertion that get_value() was invoked, the tests do not actually prove the implementation
uses the new access path and therefore would not reliably catch a regression back to root_data.

CLAUDE.md: Meet Minimum Test Coverage (90%) and Add Tests for New Code
webhook_server/tests/test_github_repository_settings.py[724-733]
webhook_server/tests/test_github_repository_settings.py[724-754]
webhook_server/tests/test_github_repository_settings.py[757-783]
webhook_server/tests/test_github_repository_settings.py[786-819]
webhook_server/tests/test_github_repository_settings.py[821-849]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Tests updated alongside the `Config.get_value()` change do not actually verify that `get_value("github-app-id")` is used, because they set `mock_config.get_value.return_value` but never assert the method was called.

## Issue Context
The current test setup provides both `root_data["github-app-id"]` and `get_value()` return values, and the main behavioral check (e.g., constructing `Auth.AppAuth(app_id=12345, ...)`) would pass regardless of whether production code reads from `root_data[...]` or calls `get_value()`. This means the tests won’t fail if the implementation accidentally reverts to reading `config_.root_data["github-app-id"]` directly.

## Fix Focus Areas
- webhook_server/tests/test_github_repository_settings.py[724-754]
- webhook_server/tests/test_github_repository_settings.py[757-783]
- webhook_server/tests/test_github_repository_settings.py[786-819]
- webhook_server/tests/test_github_repository_settings.py[821-849]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Description

get_github_app_slug() holds a module-level threading.Lock while performing the network call
app_instance.get_app().slug, so concurrent webhooks can tie up multiple asyncio.to_thread workers
waiting on the lock during the initial cache-miss window. This can delay unrelated GitHub API calls
that also rely on github_api_call()'s shared threadpool executor.

Code

webhook_server/utils/github_repository_settings.py[R458-466]

+    with _github_app_slug_lock:
+        # Double-checked locking
+        if _github_app_slug_cache is not None:
+            return _github_app_slug_cache
+
+        LOGGER.debug("Getting GitHub App slug")
+        app_instance = _create_github_integration(config_)
+        slug = app_instance.get_app().slug
+        if not slug:

Evidence

The slug fetch is performed inside a locked section and is invoked via github_api_call(), which
executes work in the shared asyncio.to_thread threadpool. During the initial cache miss, concurrent
webhooks can therefore occupy multiple threadpool workers waiting for the lock while one worker
performs the network call.

webhook_server/utils/github_repository_settings.py[446-469]
webhook_server/utils/github_retry.py[65-126]
webhook_server/libs/github_api.py[546-560]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`get_github_app_slug()` currently acquires `_github_app_slug_lock` and then performs the potentially slow GitHub API request `app_instance.get_app().slug` while holding the lock. When many webhook handlers hit this code concurrently (before the cache is populated), multiple `asyncio.to_thread` workers can block on that lock and reduce overall throughput.

### Issue Context
- `GithubWebhook.process()` initializes `app_bot_login` by calling `github_api_call(get_github_app_slug, ...)`.
- `github_api_call()` runs the callable in the default threadpool via `asyncio.to_thread`, so blocked threads reduce capacity for other GitHub calls.

### Fix Focus Areas
- webhook_server/utils/github_repository_settings.py[446-469]

### Suggested fix
Minimize the lock scope by doing the network fetch outside the lock and using the lock only to publish the cached value. A common pattern:
1) Fast path: return cached value if present.
2) Compute `slug = ...get_app().slug` without holding `_github_app_slug_lock`.
3) Acquire lock and set cache only if still unset (another thread may have already populated it).

This avoids holding the lock across network I/O while keeping the cache semantics intact.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Description

get_github_app_slug() caches a single slug in a module-global variable without keying by Config
(github-app-id / data_dir), so a later call with a different Config can return the wrong slug and
generate an incorrect app_bot_login.

Code

webhook_server/utils/github_repository_settings.py[R443-446]

+    global _github_app_slug_cache
+
+    if _github_app_slug_cache is not None:
+        return _github_app_slug_cache

Evidence

The implementation returns a single cached slug regardless of which Config is passed, even though
the slug is derived from config-provided app id and key path; Config instances can legitimately
differ because data_dir is environment-derived and app id is read from config.yaml (which may vary
across configs/repositories/tests).

webhook_server/utils/github_repository_settings.py[45-46]
webhook_server/utils/github_repository_settings.py[436-456]
webhook_server/libs/config.py[13-22]
webhook_server/libs/github_api.py[862-866]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`get_github_app_slug()` stores a single process-wide `_github_app_slug_cache` and returns it for all calls once set. If the same process ever uses multiple `Config` instances with different `github-app-id` and/or different `data_dir` (different private key/config), the cached slug can be wrong, producing an incorrect `{slug}[bot]` login.

## Issue Context
The function already takes `config_` and reads both `config_.data_dir` and `config_.root_data["github-app-id"]`, which implies the returned value is config-dependent. A process-wide singleton cache breaks that implied contract.

## Fix Focus Areas
- webhook_server/utils/github_repository_settings.py[46-46]
- webhook_server/utils/github_repository_settings.py[443-447]
- webhook_server/utils/github_repository_settings.py[454-455]

## Suggested fix
- Change `_github_app_slug_cache` from `str | None` to a dict keyed by something like `(github_app_id, config_.data_dir)` (or `(github_app_id, config_.config_path)`), and cache per-key.
- Alternatively, refactor into a helper `@functools.lru_cache` function that accepts hashable inputs (e.g., `app_id: int, pem_path: str`) and have `get_github_app_slug(config_)` call it.
- Ensure behavior remains: raise on failure so `github_api_call()` can apply retry/backoff.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Description

get_github_app_slug() reads the private key and calls GitHub (GET /app) every time it’s invoked,
and GithubWebhook.process() invokes it per webhook because app_bot_login is reset on each new
handler instance. This adds avoidable per-event I/O/API latency and can amplify transient failures
into repeated error logs under high webhook volume.

Code

webhook_server/utils/github_repository_settings.py[R441-447]

+    with open(os.path.join(config_.data_dir, "webhook-server.private-key.pem")) as fd:
+        private_key = fd.read()
+
+    github_app_id: int = config_.root_data["github-app-id"]
+    auth: AppAuth = Auth.AppAuth(app_id=github_app_id, private_key=private_key)
+    app_instance: GithubIntegration = GithubIntegration(auth=auth)
+    return app_instance.get_app().slug

Evidence

A new webhook handler instance is created for each request, and it resets app_bot_login to an
empty string, which causes process() to call get_github_app_slug() on every webhook. The helper
function performs both disk I/O (PEM read) and a GitHub API call (get_app()) each time, so the
overhead repeats per event.

webhook_server/app.py[455-461]
webhook_server/libs/github_api.py[200-200]
webhook_server/libs/github_api.py[545-553]
webhook_server/utils/github_repository_settings.py[435-447]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`get_github_app_slug()` performs disk I/O (reads the PEM) and a network call (`GithubIntegration(...).get_app().slug`) every time it’s called. Because a new `GithubWebhook` object is created per incoming webhook and `app_bot_login` starts empty, this slug lookup runs on every webhook.

## Issue Context
- `GithubWebhook` is instantiated per request.
- `self.app_bot_login` is initialized to `""` in `__init__`, so `process()` always triggers the slug lookup path.

## Fix Focus Areas
- webhook_server/utils/github_repository_settings.py[435-447]
- webhook_server/libs/github_api.py[545-553]

## Suggested fix
- Cache the app slug (or the derived bot login string) at process-level scope (module global) or application scope:
 - Option A: Introduce a module-level cache (e.g., `_APP_SLUG: str | None`) + a lock to ensure only one fetch occurs concurrently.
 - Option B: Store the slug in `Config` (or another long-lived singleton) after the first successful fetch.
- Ensure cache invalidation is either unnecessary (slug is effectively immutable) or explicitly tied to config changes (e.g., app id / data_dir).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Description

process() calls github_api_call(_github_app_api.get_app) but then reads _app.slug outside the
wrapper; if slug is lazily loaded by PyGithub, it can perform synchronous HTTP on the event loop
thread and bypass github_api_call retries/logging. Keep the entire bot-login derivation inside
github_api_call (e.g., return the final string from the wrapped callable).

Code

webhook_server/libs/github_api.py[R554-559]

+                    _app = await github_api_call(
+                        _github_app_api.get_app,
                        logger=self.logger,
                        log_prefix=self.log_prefix,
                    )
+                    self.app_bot_login = f"{_app.slug}[bot]"

Evidence

The project’s retry wrapper explicitly calls out using lambdas for property access because PyGithub
is synchronous, and other parts of the code consistently wrap attribute reads
(pull_request.number, pull_request.user.login) with github_api_call for that reason. The new
code returns an App object from the wrapper but then reads .slug outside of it, which breaks that
pattern and can bypass the wrapper’s thread offload/retry if .slug triggers lazy loading.

webhook_server/libs/github_api.py[543-565]
webhook_server/utils/github_retry.py[1-27]
webhook_server/utils/github_retry.py[65-104]
webhook_server/libs/github_api.py[622-630]
webhook_server/libs/handlers/issue_comment_handler.py[752-756]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`_app.slug` is accessed outside `github_api_call`, which is the project’s wrapper intended to keep all PyGithub (sync) operations off the event loop and apply retry behavior.

### Issue Context
This repo documents using `github_api_call` for both method calls and property access because PyGithub is synchronous and properties may require blocking work.

### Fix Focus Areas
- webhook_server/libs/github_api.py[543-565]

### Suggested change
Refactor to compute the final bot login inside `github_api_call`, e.g.:

```py
self.app_bot_login = await github_api_call(
   lambda: f"{_github_app_api.get_app().slug}[bot]",
   logger=self.logger,
   log_prefix=self.log_prefix,
)
```

(Or fetch `slug` via `github_api_call(lambda: _github_app_api.get_app().slug, ...)` and then format the string.)

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

• ➕ Avoids any GitHub API call during initialization
• ➕ Works even if GitHub API is partially unavailable
• ➖ Operationally brittle (must match app slug exactly)
• ➖ Harder to deploy across multiple environments/apps

• ➕ Potentially preserves behavior for non-app tokens in shared code paths
• ➖ Extra API call and noisy failures for installation tokens (known 403 behavior)
• ➖ More complex control flow without adding real value for the target scenario